-
Notifications
You must be signed in to change notification settings - Fork 353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #922 and uncover additional bugs #1487
Conversation
1488965
to
26f10ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only reviewed the first few commits yet. I'll try to finish it tomorrow. Thanks again for working on this!
0eae94b
to
2907628
Compare
30688f0
to
7a94558
Compare
The PR was updated to set up a special proto. The old version is at https://github.com/ilyagr/jj/tree/export-old. It's been reorganized some, especially in the first several commits, but is quite spiritually similar to what it was before. |
4d6c281
to
4360256
Compare
Thank you! I'm glad you got it to work. Sorry that I didn't think about it earlier. |
I'm actually glad I tried the version with the |
Also, I added locking. Turns out you've already done all the work. 🎉 |
// TODO 2: Conceptually, this state should contain all the information in the | ||
// repo that's not stored by git and should not be affected by `jj undo`. In | ||
// particular, jj's last seen view of remote-tracking branches should be stored | ||
// here. It is currently affected by `undo`, which can cause minor issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that the file should contain all the information in the repo that is also stored by git, i.e. the information we need to sync back and forth. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant to do here is to contrast information like the contents of commits -- for which the git repo is authoritative and the only place they are stored -- and the "last seen" git state (which not literally stored by git, as it is different from the current git state)
I'm thinking about how to clarify this comment. Perhaps we should say something about storing information that can be potentially conflicted between jj and git. That's what makes it necessary to sync it back and forth, as you said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this:
Conceptually, this state should contain all the information in the
repo that can be updated by either git or jj. For example, branches should
be stored here, but remote-tracking branches and other git refs should not.
Actually, what problem does "It is currently affected by undo
, which can cause minor issues." refer to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what problem does "It is currently affected by undo, which can cause minor issues." refer to?
Mainly, jj undo
after jj git fetch/push
can move remote tracking branches. If we use the remote tracking branches for --force-with-lease
, it could become a bigger problem.
An example that I mean to turn into a test (unless I already have):
jj git push --branch a
jj branch s a -r somewhere
jj git push --branch a
jj op restore @--
At this point, another jj git push --branch a
should push the branch a
, but I think it won't because it will consider the branch unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, another jj git push --branch a should push the branch a, but I think it won't because it will consider the branch unchanged.
IIRC, git push
doesn't check the 'local' value of the remote branch (refs/remotes/my-remote/my-branch on the local clone); it only checks the 'remote' value of the remote branch (refs/heads/my-branch on the remote). So if jj git push
replicates git push
closely (that depends on libgit2 and whether we added extra steps), it should push the branch.
Ah sorry, I forgot about the --force-with-lease
argument, in which case, the 'local' value is checked, yes. I agree that that would be confusing.
// TODO 2.5: If remote branches were stored here, they should no longer be | ||
// stored in jj's normal View and the format of the file would no longer be a | ||
// View object. This would allow us to treat the state exported to the git repo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this file as a way of detecting what has changed in git since last time. I suppose you can think of it much like a merge base used when merging git's current state with jj's current state. So we would still need to store the current state in the view, I think.
I think we can consider removing the git_refs
we track, but that's a separate question of cost/benefit (do users care enough about seeing them in log output?). I don't want to remove the tracking of remote branches in general, especially since that's not specific to the git backend, so we can't rely on git's state.
/// Reads the last seen state of git refs from a file, updates it with the | ||
/// provided function, and writes the result back to disk. | ||
/// | ||
/// The state currently tracks only the local branches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we need to track remote-tracking branches too? Since they can't be modified directly (only via import and undo/restore), I feel like we shouldn't track them and instead consider the Git repo the source of truth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This became a confusing question for me once I started thinking about the issue of concurrency and distributed filesystems, where we can't really trust git. Ignoring those questions, you are probably right.
However, if a repo is on Dropbox and the remote-tracking branch in git gets corrupted, the current behavior of creating a conflict is nice. Or should we just lose the remote tracking branch position in that case? This becomes more important when we try to do something like --force-with-lease
.
I'm also not sure if the same answer is good for colocated and non-colocated repositories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what you said here seems to contradict what you said in #1487 (comment). Does this mean you are in the same uncertain position I am, or did I misunderstand your point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is confusing, but I don't see the contradiction between these two comments. I think both are saying that we should store our own concept of remote-tracking branches in the view object but probably not in the new file introduced by this PR.
if a repo is on Dropbox and the remote-tracking branch in git gets corrupted, the current behavior of creating a conflict is nice
Yes, I agree. Ah, it was probably my "Do you think we need to track remote-tracking branches too?" that sounded like I was saying that we shouldn't even track them in our model? I meant it in the context of the new file introduced by this PR, i.e. that I don't think we need to track remote-tracking branches in that file.
Makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounded like I was saying that we shouldn't even track them in our model?
Yes, that's what I was thinking about.
I don't think we need to track remote-tracking branches in that file. Makes sense?
I think it makes sense in practice; that's what the reality would be after this PR. I'm not 100% sure it works in theory :). In other words, it makes sense in the short term, and I'm still confused about the long term.
Mostly, that's because I was hoping that we could have a framework where exported branches and remote-tracking branches are treated in the same way. That may simply be a flawed idea, but I haven't adjusted my mind to a consistent theory of how it should work otherwise. I guess we forget jj's view of remote tracking branches whenever there is a conflict or git loses this information and wait for the next fetch. Hopefully this will work OK for our version of --force-with-lease
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I'm saying in the last paragraph is that I'm not sure there's a behavior that keeps the remote-tracking branches in jj's model, makes sense, doesn't track the conflict base for conflits between jj and git, works with undo (or can we compromise here?), and generates benefit compared to only tracking them in git. I'm also not sure it doesn't exist. Finally, I'm additionally not sure how the current jj behavior measures up on the last two points.
570de0d
to
e0d297b
Compare
149b273
to
599cc7c
Compare
The only change of behavior here is that an empty `last_seen_git_refs` file is created on export. It is also read but ignored. The goal is to encapsulate the changes in behavior in the next commit.
…file The lock is called "git_refs.lock" since it should also protect against concurrent modification of refs in the backing Git repo.
…-vcs#922 This commit essentially reverts 8a440d8 AKA jj-vcs@4dfd765 The problem that commit is that the Git repo is, unaffected by `jj undo`. So, it is important for JJ's view of the git refs to be unaffected by `jj undo`. Fixes jj-vcs#922. Also creates a bug that is fixed in a follow-up commit
This TODO may address the bugs demoed by subsequent commits.
If it doesn't exist, generate it from git refs as jj did before. This should work as long as the last operation wasn't "undo" or "restore". This is a temporary behavior designed for people upgrading from older jj. It may also be useful if the file gets corrupted by concurrent operations that ignore the lock (e.g. Dropbox). It's unclear if it should be kept or removed.
This will make it more similar to the test introduced in the next commit.
At this point, it works more or less correctly. However, in the next commit adding another `jj push` breaks this test.
…-undo test This change should not affect the test at all, but does.
…push This reveals another bug I found.
All parts of this PR are now obsolete:
|
Fixes #922.
This is not a complete fix, some of the commits demonstrate bugs that are yet to be fix. However, in practice this helps a lot.
This is a little unfinished, but the rest can be done in subsequent PRs (or in this one, if people prefer). In any case, it seems ready to review. For example, this PR mostly changes
jj git export
and only changesjj git import
a little.See https://github.com/martinvonz/jj/labels/colocated for yet more bugs to fix.
Checklist
If applicable:
CHANGELOG.md